Rewrite legend visibility toggling #2022

rreusser · 2017-09-19T23:20:00Z

Legend visibility toggling didn't work at all with groupby, which led to mostly a rewrite. By happenstance, it toggled them as a group, which wasn't really the desired behavior. More problematically though, it also threw lots of Uncaught bad container errors because it really just wasn't accounted for at all. This PR rewrites legend visibility toggling and tries to preserve existing behavior while fixing groupby.

In particular, one tricky case is legendgroup, both toggling and isolating:

And the other is groupby transforms, both toggling and isolating:

rreusser · 2017-09-19T23:28:21Z

Oh, and to preempt the question, the answer is, "Yes." 😄

(The legend should say legendgroup, not just group. To not be so cute, the question is: "Does it work with legendgroups spread across different groupby groups?")

etpinard · 2017-09-20T13:47:49Z

src/components/legend/draw.js

+                case false:
+                    nextVisibility = false;
+                    break;
+                default:


🔪 that default: ?

etpinard · 2017-09-20T14:31:41Z

src/components/legend/draw.js

+            }
+
+            // If not specified, assume visible:
+            var curState = carr.get(fullTrace._group) || true;


Why does this happen?

this === that || true fallback.

Good catch. This isn't a great conditional. If it's really fullData, it should always be present and what it means to be. I think I'll just remove this.

Haha couldn't remove it. Modified it to be a bit more targeted/explicit. The issue is that the container doesn't always have visible defined. "Oh, so just use fullTrace.visible." That didn't work either. Broke some tests. Aimed for some middle ground that passes all the tests.

etpinard · 2017-09-20T14:32:48Z

src/components/legend/draw.js

+            if(!carr) {
+                var groupbyIndices = Registry.getTransformIndices(fullInput, 'groupby');
+                var lastGroupbyIndex = groupbyIndices[groupbyIndices.length - 1];
+                carr = Lib.keyedContainer(fullInput, 'transforms[' + lastGroupbyIndex + '].styles', 'target', 'value.visible');


keyedContainer for the win 🏆

By the way, is carr short for container array?

... no need to rename carr to something more explicit, I'm just curious.

Yes, it's somewhat of a holdover. It's not great. Maybe kcont?

carr or kcont, your call.

kcont. ✅

etpinard · 2017-09-20T14:34:23Z

src/components/legend/draw.js

+            }
+            carrIdx[fullInput.index] = insertUpdate(fullInput.index, 'visible', fullInput.visible === false ? false : true);
+        } else {
+            // false -> false (not possible since will not be visible in legend)


etpinard · 2017-09-20T14:40:45Z

src/components/legend/draw.js

+
+            if(hasLegendgroup) {
+                for(i = 0; i < fullData.length; i++) {
+                    if(fullData[i].visible && fullData[i].legendgroup === legendgroup) {


maybe fullData[i].visible !== false instead of fullData[i].visible would be best, to make things more explicit. It is easy to forget that visible supports two truthy values.

etpinard · 2017-09-20T14:43:05Z

src/components/legend/draw.js

+            // Compute the clicked index. expandedIndex does what we want for expanded traces
+            // but also culls hidden traces. That means we have some work to do.
+            var clickedIndex, isIsolated, isClicked, isInGroup, otherState;
+            for(clickedIndex = 0; clickedIndex < fullData.length; clickedIndex++) {


Isn't clickedIndex simply fullTrace.index?

Let's see… it might be. I need to think about this just a bit. fullTrace.index vs. fullTrace._fullInput.index vs. fullTrace._expandedIndex obey slightly different rules. I think more often that not the answer was to avoid at all costs needing the index at all. I'll give the above a try.

Ah, good catch. clickedIndex isn't actually used at all, which was the end result of trying to get this right (i.e. don't use it at all). I've removed this code.

etpinard · 2017-09-20T14:47:00Z

src/components/legend/draw.js

@@ -520,52 +570,116 @@ function handleClick(g, gd, numClicks) {

        Plotly.relayout(gd, 'hiddenlabels', hiddenSlices);


I think we can return early after Plotly.relayout for pie traces.

Ignore ⤴️ I misread the if block levels.

etpinard · 2017-09-20T14:49:22Z

src/components/legend/draw.js

+        // The length of the value arrays should be equal and any unspecified
+        // values should be explicitly undefined for them to get properly culled
+        // as updates and not accidentally reset to the default value. This fills
+        // out sparse arrays with the required number of undefined values:


Very nice touch 👌

etpinard · 2017-09-20T14:58:09Z

Very lovely PR ❤️ with strong tests ⚒️

I made a few comments; I'll make a second review after reading your answers.

As an aside, legend/draw.js now probably has more click & edit logic than actual drawing logic. Do you think factoring out click & edit logic into a separate module (i.e. a few methods that take in clicked data and return restyle/relayout update objects) would make the legend code easier to maintain?

rreusser · 2017-09-20T17:15:14Z

Okay, I've made these changes. And since most of the other code was related to drawing, I've moved the click handler to a separate file. I left text editing in place since it's moderately short, though it could also be moved.

etpinard · 2017-09-20T17:41:06Z

src/components/legend/draw.js

+            // ways to do this (e.g. why not just `curState = fullTrace.visible`??? The
+            // answer is: because it breaks other things like groupby trace names in
+            // subtle ways.)
+            if(curState === undefined) {


Thanks for the comment 📚

etpinard · 2017-09-20T18:14:43Z

💃 thanks very much @rreusser 🎉

rreusser added 2 commits September 19, 2017 16:10

Rewrite legend visibility toggling

5cc7dc6

Lint-fix

6b8dd49

rreusser mentioned this pull request Sep 19, 2017

Legend toggles visibility of all groupby groups #2013

Closed

Add overlooked test

f17d763

etpinard added status: reviewable bug something broken labels Sep 20, 2017

etpinard reviewed Sep 20, 2017

View reviewed changes

rreusser added 3 commits September 20, 2017 10:02

Fix small PR review issues for legend interactions

c729353

Remove unused code in legend interactions

f8a78a2

Move legend click handler to separate file

d23c24d

etpinard reviewed Sep 20, 2017

View reviewed changes

rreusser merged commit d78b965 into master Sep 20, 2017

rreusser deleted the legend-visibility-toggle-rewrite branch September 20, 2017 19:01

etpinard mentioned this pull request Sep 20, 2017

Isolating a trace makes visible:false traces visible:legendonly #2019

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite legend visibility toggling #2022

Rewrite legend visibility toggling #2022

rreusser commented Sep 19, 2017 •

edited

Loading

rreusser commented Sep 19, 2017 •

edited

Loading

etpinard Sep 20, 2017 •

edited

Loading

rreusser Sep 20, 2017

etpinard Sep 20, 2017

etpinard Sep 20, 2017

rreusser Sep 20, 2017

rreusser Sep 20, 2017

etpinard Sep 20, 2017

etpinard Sep 20, 2017

rreusser Sep 20, 2017

etpinard Sep 20, 2017 •

edited

Loading

rreusser Sep 20, 2017

etpinard Sep 20, 2017

etpinard Sep 20, 2017

rreusser Sep 20, 2017

etpinard Sep 20, 2017

rreusser Sep 20, 2017

rreusser Sep 20, 2017

etpinard Sep 20, 2017

etpinard Sep 20, 2017 •

edited

Loading

etpinard Sep 20, 2017

etpinard commented Sep 20, 2017

rreusser commented Sep 20, 2017

etpinard Sep 20, 2017

etpinard commented Sep 20, 2017

		@@ -520,52 +570,116 @@ function handleClick(g, gd, numClicks) {

		Plotly.relayout(gd, 'hiddenlabels', hiddenSlices);

Rewrite legend visibility toggling #2022

Rewrite legend visibility toggling #2022

Conversation

rreusser commented Sep 19, 2017 • edited Loading

rreusser commented Sep 19, 2017 • edited Loading

etpinard Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Sep 20, 2017

rreusser commented Sep 20, 2017

Choose a reason for hiding this comment

etpinard commented Sep 20, 2017

rreusser commented Sep 19, 2017 •

edited

Loading

rreusser commented Sep 19, 2017 •

edited

Loading

etpinard Sep 20, 2017 •

edited

Loading

etpinard Sep 20, 2017 •

edited

Loading

etpinard Sep 20, 2017 •

edited

Loading